Skip to content

security(graduation): enforce manager-only assessment approval server-side#267

Open
gonzalesedwin1123 wants to merge 4 commits into
19.0from
security-graduation-self-approval
Open

security(graduation): enforce manager-only assessment approval server-side#267
gonzalesedwin1123 wants to merge 4 commits into
19.0from
security-graduation-self-approval

Conversation

@gonzalesedwin1123

Copy link
Copy Markdown
Member

Do not merge yet — held for human review.

Summary

spp.graduation.assessment gated its approve/reject/reset buttons to group_spp_graduation_manager in the view only. The Python workflow methods checked only the state transition, and group_spp_graduation_user has write/create on their own assessments (record rule assessor_id == user.id). So a normal graduation user could, via RPC, self-approve their own assessment (action_submit then action_approve, which sets a graduation_date) — or bypass the methods entirely with a raw write({'state':'approved', ...}) or create({'state':'approved', ...}) — crossing the intended manager approval boundary and corrupting beneficiary graduation status.

Fix (server-side, two layers)

  • Action guards: action_approve/action_reject/action_reset_draft raise AccessError unless the caller is a graduation manager (_is_graduation_manager() = manager group, with superuser/sudo exempt so system/programmatic flows aren't blocked). action_submit stays user-allowed.
  • create() / write() guards: a non-manager may not set the manager-only outcome fields (approved_by_id, approved_date, graduation_date) and may only move state draft → submitted (create must be draft; no-op same-state writes allowed). This closes the raw-RPC bypass that the action guards alone would miss.

Tests

  • Blocked (as user): action_approve/reject/reset_draft on own assessment; raw write of state='approved' / graduation_date / approved_by_id; create of a non-draft assessment; un-submit (submitted→draft); create for a different assessor.
  • Still works: manager approve (sets state='approved', approved_by_id, graduation_date); user submit; user editing recommendation/notes on their own draft.
  • Green: spp_graduation 57/57; lint/semgrep clean.

Reviewed

Adversarial staff review: APPROVE, no blockers. Confirmed no unguarded write path, no sudo/automation escape, and that the downstream spp_case_graduation only treats a beneficiary as graduated when state=='approved' and graduation_date is set (both manager-only) — so recommendation/scores are not levers. The three review test-gap suggestions were added.

spp.graduation.assessment gated its approve/reject/reset buttons to the
graduation manager group in the VIEW only. The Python workflow methods checked
just the state transition, and group_spp_graduation_user has write/create on
their own assessments (assessor_id == user). So a normal user could, via RPC,
self-approve their own assessment (action_submit then action_approve, setting a
graduation_date) or bypass the methods entirely with a raw
write({'state':'approved', ...}) / create({'state':'approved', ...}).

Enforce the boundary server-side:
- action_approve/action_reject/action_reset_draft raise AccessError unless the
  caller is a graduation manager (superuser/sudo exempt for system flows).
- create()/write() forbid non-managers from setting the manager-only outcome
  fields (approved_by_id, approved_date, graduation_date) and from moving state
  beyond the one user-allowed transition draft -> submitted (create must be
  draft). Closes the raw-RPC bypass.
action_submit stays user-allowed.

Tests: user cannot approve/reject/reset own assessment, cannot write approved
state or approval fields, cannot create a non-draft assessment; manager can
still approve (graduation_date set); user can still submit. spp_graduation 54/54.
Post-review defense-in-depth tests: a user can still edit recommendation/notes
on their own draft (guard does not over-block); a user cannot un-submit
(submitted -> draft is manager-only); a user cannot create an assessment for a
different assessor (record-rule protection). spp_graduation 57/57.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces robust security restrictions to prevent non-manager users from approving, rejecting, resetting, or directly modifying workflow-related fields on graduation assessments at the ORM level. It also adds comprehensive unit tests to verify these boundaries. The review feedback suggests further strengthening security by preventing non-managers from modifying any fields on assessments that are not in the draft state, and adding a corresponding test case to verify this behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread spp_graduation/models/graduation_assessment.py
Comment thread spp_graduation/tests/test_graduation_security.py
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.34%. Comparing base (f260ffd) to head (c2a6e69).
⚠️ Report is 9 commits behind head on 19.0.

Files with missing lines Patch % Lines
spp_graduation/models/graduation_assessment.py 97.22% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             19.0     #267      +/-   ##
==========================================
+ Coverage   75.31%   75.34%   +0.03%     
==========================================
  Files        1098     1098              
  Lines       65317    65391      +74     
==========================================
+ Hits        49191    49269      +78     
+ Misses      16126    16122       -4     
Flag Coverage Δ
spp_base_common 90.26% <ø> (ø)
spp_case_graduation 97.77% <ø> (ø)
spp_graduation 98.80% <97.22%> (+5.19%) ⬆️
spp_programs 65.27% <ø> (ø)
spp_registry 86.83% <ø> (ø)
spp_security 66.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
spp_graduation/__manifest__.py 0.00% <ø> (ø)
spp_graduation/models/graduation_assessment.py 99.17% <97.22%> (+7.94%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Address Gemini review (security-high) on PR #267: the ORM guards blocked state
changes and the manager-only outcome fields, but a non-manager assessor could
still edit content (recommendation, partner_id, responses, ...) on an already
submitted/approved/rejected assessment — e.g. submit recommendation='extend',
flip to 'graduate' while pending, and the manager's approval then sets a
graduation_date for a recommendation they did not review.

Freeze the assessor-editable content fields (partner_id, pathway_id,
assessment_date, recommendation, recommendation_notes, response_ids) for
non-managers once the assessment leaves draft; editing requires a manager reset
to draft. Only these business fields are frozen, so chatter/activity/technical
writes on submitted records are unaffected. Manager/superuser unrestricted.

Test: a user cannot change recommendation/notes on a submitted assessment.
spp_graduation 58/58.
Record the manager-only assessment approval hardening in the version and
HISTORY changelog (regenerated README).
@gonzalesedwin1123 gonzalesedwin1123 force-pushed the security-graduation-self-approval branch from 17276e3 to c2a6e69 Compare July 2, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant